feat: feat(cluster-protocol): publish the TypeScript cluster client with full v1 parity - #799
feat: feat(cluster-protocol): publish the TypeScript cluster client with full v1 parity#799tomdps wants to merge 1 commit into
Conversation
… cluster client with full v1 parity
Greptile SummaryAdds a publishable TypeScript v1 Cluster Protocol client.
Confidence Score: 3/5The PR should not merge until watch reconnection uses the fresh connection and request IDs cannot collide between clients sharing a transport. A real WebSocket reconnect sends the replacement watch over the disconnected transport, while independently constructed clients can also reject valid concurrent calls because their per-client counters produce duplicate IDs in one transport-wide pending map. Files Needing Attention: src/cluster/watch-subscription.ts, src/cluster/cluster-client.ts, src/cluster/multiplex.ts
|
| Filename | Overview |
|---|---|
| src/cluster/watch-subscription.ts | Implements deduplicated watch streams, but reconnect mixes the supplied fresh client with the original closed subscription transport. |
| src/cluster/cluster-client.ts | Implements typed unary calls and cancellation, but allocates IDs per client despite transports being shareable. |
| src/cluster/multiplex.ts | Adds request/subscription demultiplexing and bounded queues; its transport-wide pending map exposes per-client request-ID collisions. |
| src/cluster/websocket-transport.ts | Adds the Node/browser WebSocket binding and connects it to the multiplexed transport. |
| package.json | Publishes the cluster subpath and adds generation, build, test, and lifecycle scripts. |
| scripts/generate-cluster-protocol-ts.js | Generates TypeScript protocol projections and supports deterministic drift checking. |
Sequence Diagram
sequenceDiagram
participant App
participant OldWatch as Original watch transport
participant FreshClient as Supplied ClusterClient
participant FreshSocket as Fresh transport
App->>OldWatch: stream.reconnect(FreshClient)
OldWatch->>FreshClient: get(atCursor)
FreshClient->>FreshSocket: JSON-RPC get
FreshSocket-->>FreshClient: coherent snapshot
FreshClient-->>OldWatch: snapshot
OldWatch->>OldWatch: establishWatch(this.transport)
OldWatch--xOldWatch: send through closed original socket
OldWatch-->>App: reconnect rejects
Reviews (1): Last reviewed commit: "feat: implement #748 - feat(cluster-prot..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| runId: this.runId, | ||
| fromCursor: snapshot.atCursor ?? this.lastDelivered, | ||
| }; | ||
| return establishWatch(this.transport, params, this.seen); |
There was a problem hiding this comment.
When the original WebSocket disconnects and the caller supplies a client using a freshly dialed connection, get() uses the fresh transport but establishWatch(this.transport, ...) sends the replacement watch through the original closed socket, causing reconnect() to reject instead of restoring the stream.
Context Used: AGENTS.md (source)
|
|
||
| export class ClusterClient { | ||
| private readonly transport: JsonRpcTransport; | ||
| private nextId = 1; |
There was a problem hiding this comment.
When two ClusterClient instances share one transport and have overlapping requests at the same sequence number, both allocate the same ID from their per-client counters. The transport-wide pending map rejects the second valid request as already pending instead of sending it.
|
CI and external review found three release-blocking defects: the package architecture test fails because the cluster guard comment names the provider-helper source; reconnect opens the replacement watch on the closed original transport; and per-client request counters collide when clients share one multiplexed transport. Closing this attempt so a fresh Zeroshot run can repair the contract from current dev. |
…ngine#843) Closes the-open-engine#748 ## Outcome Publishes `@the-open-engine/zeroshot/cluster` as a Node 18+ CJS/ESM/TypeScript WebSocket client for the full v1 cluster protocol. ## Ownership decisions - The connection owns collision-free request IDs, pending entries, exact failed-send cleanup, bounded frame decoding, response routing, and OPEN/CLOSING/CLOSED teardown. - Every subscription uses the shared 1024-entry bounded multi-waiter queue; overflow unregisters locally, cancels remotely best-effort, clears retained frames, and returns the protocol slow-consumer close. - Durable watch owns one shared reconnect attempt, reconstructs state via coherent `get` then `watch(fromCursor)`, and suppresses the last ordered physical `(runId,cursor)` redelivery with constant memory across reconnects. - Abort/iterator return detach their exact listeners and cancel exactly once; logs and attach expose terminal closure without a recoverable cursor. - Wire types, method/result maps, validators, constants, and the embedded schema are generated deterministically from checked-in Rust/OpenRPC artifacts. There are no hand-maintained DTOs. - The package exports preserve the root/deep-import surface while adding the built `cluster` subpath, optional installed `ws` runtime, and injectable browser-compatible factory. The issue's two ownership-design comments were followed directly. The claimed checked-in `docs/v2/748-typescript-client-design.md` was absent from fresh `origin/main`, so no replacement design document was fabricated. ## Regression coverage Deterministic tests cover every prior the-open-engine#799-the-open-engine#802 defect class: shared-ID collisions, exact pending cleanup after failed sends, 1024-entry overflow for all stream types, failed/competing reconnect ownership, close at every reconnect boundary, multi-waiter FIFO, close cleanup after cancel failure, post-handshake abort, malformed peer frames and bounded diagnostics, generated response validation, and packed CJS/ESM/TypeScript/default-`ws` consumers. External review added and closed two security P1s before merge: durable-watch dedup state is scalar/constant-memory over thousands of unique events, and inbound binary/text byte limits are enforced before decode/encoding-copy allocation. Both review threads include fix and independent probe evidence. Independent non-authoring verifiers required four rounds. Their final runs reproduced the historical races plus malformed-frame, error-envelope, cancellation, installed-runtime, 5,000-event dedup, UTF-8 boundary, and decoder-allocation probes. No P0/P1/security finding remains. ## Verification - `npm run protocol:check` - `npm run typecheck` - `npm run lint` (0 errors; existing warnings remain) - `npm run test:cluster-client` — 38/38 - `npm run test:cluster-package` — 4/4 - `npm test` — 1953 passing, 18 pending - `npm pack --dry-run` - `npm run rust:check` - `npm run test:unit` — 1953 passing, 18 pending - `npm run check:agent-cli-provider:ci` — 147/147 - `npm run dupcheck` - `opcore check --changed` ## Non-goals preserved No Rust dispatcher/server/client behavior, in-process TypeScript transport, hosted authentication, provisioning, or native-daemon behavior changed. ## Discovered protocol work `WatchResult` uniquely lacks Rust `#[serde(deny_unknown_fields)]`, so its generated schema permits unknown properties while `LogsResult` and `AgentAttachResult` are closed. This PR intentionally follows generated authority rather than hand-tightening the TypeScript validator.
1 participant
Closes #748